Skip to content

Modified the GuessDelimiter function.#593

Open
ErikSimonsen wants to merge 11 commits into
mholt:masterfrom
ErikSimonsen:master
Open

Modified the GuessDelimiter function.#593
ErikSimonsen wants to merge 11 commits into
mholt:masterfrom
ErikSimonsen:master

Conversation

@ErikSimonsen

Copy link
Copy Markdown

Changed the GuessDelimiter function, so that a new delimiter is chosen as best if its delta value is higher than the current chosen.
Added a testcase for decimal values with comma as decimal separator and pipes as delimiter.
(Which should mimic the following test data:
a|3,4|b
c|3,4|d
)

But i couldnt ran the tests because i dont have a node interpreter installed, so i am not sure if the other tests break, altough the test data mentioned above now produces the correct result).

Changed the GuessDelimiter function, so that a new delimiter is chosen as best if its delta value is higher than the current highest.
Added a testcase for decimal values with comma as decimal separator and pipes as delimiter.

But i couldnt ran the tests as i dont have a node interpreter installed, so i am not sure if the other tests dont break now.
@pokoli

pokoli commented Nov 2, 2018

Copy link
Copy Markdown
Collaborator

Hi, thanks for the PR I see you've added a new test-cases.js file, you should modify the tests-cases.js file in the test folder.

Added testcase with pipes and decimal values with commas as decimal separator.
@pokoli

pokoli commented Nov 6, 2018

Copy link
Copy Markdown
Collaborator

We use tabs for formating and you just added some code with spaces. This breaks the linter , could you please fix it?

@pokoli

pokoli commented Nov 6, 2018

Copy link
Copy Markdown
Collaborator

I will prefer to include only in this pull request the changes related to the GuessDelimiter function and move all the other code styling changes in a separate Pull Request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants